This rule wants methods inside a React class to be sorted alphabetically, and is responsible for warnings like "render must be placed after isServiceWorker". I don't really like it, because it prevents organizing methods in ways that could seem more natural/useful, e.g.: createClass({ // React-boilerplate, common in most classes: getInitialState() // also, I feel this makes more sense at the top componentDidMount() componentWillUnmount() render() // Class-specific, unlikely to be in other classes: getSomeActorForms() openSomeToolbox() sendPushNotification() }) I think the above makes a component easier to read than "every method mashed together alphabetically", and is more helpful to people who want to compare components / implement a new component.

Lin or James, I'd love your opinion on this. Is "react/sort-comp" really helpful at improving our code base?

Hmm, this rule shouldn't be doing things alphabetically. It's actually specifying an order: > "react/sort-comp": [1, { > order: [ > "propTypes", > "everything-else", > "render" > ] > }], So I'll explain the reasoning behind that. - propTypes should come first (though maybe this should change to beging second, after displayName). This makes the API that the component provides prominent and easy to see when you first look at the component. - Assuming that there are no class-specific methods, render should come last. Even though that's not chronological order with the lifecycle hooks, that's usually how I've seen it. Does anyone disagree with that? If so, we can discuss. Now we get to class-specific methods. What we've discussed in the React components meeting is trying to not have class-specific methods on the component as much as possible. We're trying to use a stateless functional approach as much as we can. Even on components that can't be stateless functional components, we're trying to avoid doing `this.` where it's not necessary. That said, you're right, we shouldn't be placing those methods above render. Totally up to change this rule, so let's use this issue to bang out what the order should be.

I like the rule if it only forces propTypes at the top and any render* methods at the bottom. That tends to be how React components are written and I'm kind of used to that. It's nice because `render` is probably the most important method so if I'm looking across components I know that I just need to look at the bottom of it to see `render`. I definitely don't think we should enforce alphabetical sorting (if it's doing that) or any sort of sorting of the other methods. This is a small style thing but it goes a long way at keeping code written by many people across many tools at least something consistent. Unfortunately adopting rules means that we can't please everybody and we all have to tweak at least something. (I'm still getting used to a few of the rules) We are trying to use functions as components, but we're going to have plenty of createClass-based ones too, so we still need to figure this out. If anything, functions might be worse because we can't actually enforce this (`render` should be at the bottom of the file). I don't know. If it's something you're finding hard to give up, we could at least get Nick and Jordan's opinion and see if there's a majority consensus.

Thanks Lin an James for replying so quickly! I probably misunderstood "sort-comp" and "render must be placed after isServiceWorker" as "please sort alphabetically", where it was actually "please respect this agreed-upon method grouping" (the latter seems more justified). (In reply to Lin Clark [:linclark] from comment #2) > > "react/sort-comp": [1, { > > order: [ > > "propTypes", > > "everything-else", > > "render" > > ] > > }], > > So I'll explain the reasoning behind that. > > - propTypes should come first (though maybe this should change to beging > second, after displayName). This makes the API that the component provides > prominent and easy to see when you first look at the component. I agree about displayName first, immediately followed by propTypes. I would then suggest other React-boilerplate things like getInitialState(), componentDidMount() and componentWillUnmount(), but let's not over-regulate this (so no eslint-enforcing). > - Assuming that there are no class-specific methods, render should come > last. Even though that's not chronological order with the lifecycle hooks, > that's usually how I've seen it. Does anyone disagree with that? If so, we > can discuss. I agree about having render() at the bottom if there are no class-specific methods. It seems that we disagree about where render() should be relative to class-specific methods: - The current "react/sort-comp" eslint rule has "everything-else" first and "render" at the bottom. - James agrees with the rule. - Lin agrees to have class-specific methods below render(). I understand that render() needs a stable and prominent place. The bottom could be just that, but you actually have to scroll past a bunch of very specific things you might not care about before reaching render(). I personally think that render() makes sense at the bottom of what I called "React boilerplate" while remaining part of that block (i.e. non-react methods come after render()). Also, the memory tool has one component with a class-specific method, and it's after render(): https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/census-tree-item.js If we count the existing rule and the memory tool example as votes, we're at 3:2 in favor of class-specific methods *after* render(). Not sure how we would re-write the eslint rule though. > Now we get to class-specific methods. What we've discussed in the React > components meeting is trying to not have class-specific methods on the > component as much as possible. We're trying to use a stateless functional > approach as much as we can. Even on components that can't be stateless > functional components, we're trying to avoid doing `this.` where it's not > necessary. That said, you're right, we shouldn't be placing those methods > above render. I don't really follow why having class-specific methods implies not being "stateless functional". In about:debugging, class-specific methods mostly serve as button click listeners, e.g. debug(), push() and start() in: https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/target.js There are also two list components which use specific methods to generate/update a list of things in their state: https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/workers-tab.js https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/addons-tab.js Maybe the latter are the more problematic in regard to being "stateless functional", but I don't see how to change that (should the lists be moved from state to props? by inlining the list-building methods into the higher level component's render() method?)

I agree with Comment 0 / Comment 2. IME render is easy to find near the top since the boilerplate above it tends to be small enough that at least part of the function is in the initial viewport.

I don't really care that much as long as we're consistent :)

It seems we agree on the following order: "react/sort-comp": [1, { order: [ "displayName", "propTypes", "react-boilerplate", "render", "everything-else" ] }] Now I just made up "react-boilerplate" but I don't know how to actually make it work, except that it can contain the following: > getInitialState > getDefaultProps > propTypes > mixins > statics > displayName > componentWillMount > componentDidMount > componentWillReceiveProps > shouldComponentUpdate > componentWillUpdate > componentDidUpdate > componentWillUnmount Source: https://facebook.github.io/react/docs/component-specs.html

(In reply to Jan Keromnes [:janx] from comment #7) > Now I just made up "react-boilerplate" but I don't know how to actually make > it work, except that it can contain the following: Well, without "render" "displayName" and "propTypes" obviously because we're specifying the position of these outside of "react-boilerplate".

After looking around a bit more, I'd like to reconsider my opinion about render. Now that I've been paying close attention, James is right, having render at the very bottom is more standard. I propose that we borrow from Khan Academy's style guide for this: https://github.com/Khan/style-guides/blob/master/style/react.md#order-your-methods-with-lifecycle-first-and-render-last 0*. displayName, propTypes 1. lifecycle methods (in chronological order: getDefaultProps, getInitialState, componentWillMount, componentDidMount, componentWillReceiveProps, shouldComponentUpdate, componentWillUpdate, componentDidUpdate, componentWillUnmount) 2. everything else 3. render *I added 0 in because they don't have those two.

(In reply to Lin Clark [:linclark] from comment #9) > After looking around a bit more, I'd like to reconsider my opinion about > render. Now that I've been paying close attention, James is right, having > render at the very bottom is more standard. > > I propose that we borrow from Khan Academy's style guide for this: > https://github.com/Khan/style-guides/blob/master/style/react.md#order-your- > methods-with-lifecycle-first-and-render-last > > 0*. displayName, propTypes > 1. lifecycle methods (in chronological order: getDefaultProps, > getInitialState, componentWillMount, componentDidMount, > componentWillReceiveProps, shouldComponentUpdate, componentWillUpdate, > componentDidUpdate, componentWillUnmount) > 2. everything else > 3. render > > *I added 0 in because they don't have those two. RDM modules are generally trying to match this ordering (might be a few outliers to fix), so it mostly sounds good to me. Should "mixins" have a spot near the top? I've been putting that up near displayName and propTypes at the moment so it's clear which components are pure[1], but I don't feel too strongly about where it ends up. [1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/components/browser.js#16

That makes sense. Here's a patch that includes mixins in the same group as displayName and propTypes.

Comment on attachment 8737226 [details] [diff] [review] Bug1258353-eslint.patch Review of attachment 8737226 [details] [diff] [review]: ----------------------------------------------------------------- I believe this looks good!

Sounds great to me!

Comment on attachment 8737226 [details] [diff] [review] Bug1258353-eslint.patch Review of attachment 8737226 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how the "lifecycle" group behaves, does it require everything in it to be sorted in that particular way? That would seem a little strict to me. Additional thought, in comment 3 James mentions "any render* methods at the bottom" rather than just "render" (e.g. the memory tool sometimes splits out "render" helpers like [0]). [0] https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/shortest-paths.js#125

Yes, it does require everything to be sorted in that particular order. For the most part we want that since that is the chronological order of the lifecycle events. My personal opinion is that helper methods shouldn't be on the component. Instead they should be independent helper functions in the same module. This makes it possible to switch to stateless functional components. However, if you want to have render helpers on the component, they could be placed above the render method. If there are no objections, I'd say let's land this so we can get moving on the issues it blocks.

(In reply to Lin Clark [:linclark] from comment #15) > If there are no objections, I'd say let's land this so we can get moving on > the issues it blocks. Agreed!

No r+, no service. Also, needs rebasing.

Comment on attachment 8741886 [details] [diff] [review] bug1258353.patch Review of attachment 8741886 [details] [diff] [review]: ----------------------------------------------------------------- > Update react/sort-comp eslint rule. Nit: Patch description probably needs "Bug 1258353 -" and "r=janx". ::: devtools/.eslintrc @@ +47,5 @@ > "react/no-unknown-property": 2, > "react/prefer-es6-class": [1, "never"], > // Disabled temporarily until errors are fixed. > "react/prop-types": 0, > "react/sort-comp": [2, { Note: 2 is error-level, so changing this rule could potentially cause new errors in our codebase. If that's the case, I'd suggest either switching it back to warning-level (1); or fix any new errors caused by the rule change; or otherwise ensure that the patch won't fail on try.

(In reply to Jan Keromnes [:janx] from comment #19) > Comment on attachment 8741886 [details] [diff] [review] > bug1258353.patch > > Review of attachment 8741886 [details] [diff] [review]: > ----------------------------------------------------------------- > > > Update react/sort-comp eslint rule. > > Nit: Patch description probably needs "Bug 1258353 -" and "r=janx". > > ::: devtools/.eslintrc > @@ +47,5 @@ > > "react/no-unknown-property": 2, > > "react/prefer-es6-class": [1, "never"], > > // Disabled temporarily until errors are fixed. > > "react/prop-types": 0, > > "react/sort-comp": [2, { > > Note: 2 is error-level, so changing this rule could potentially cause new > errors in our codebase. If that's the case, I'd suggest either switching it > back to warning-level (1); or fix any new errors caused by the rule change; > or otherwise ensure that the patch won't fail on try. Please don't land more rules at warning level (1), they just create noise and confusion on try. Either off (0) or error (2) only.

Thanks for catching that janx. I understand the concern about introducing a new warning, but introducing this at 2 would mean that this patch has to do all of the work of changing the order, which would be a coordination project that I don't have the focus for atm. I see three options: 1. We introduce it at 1 and let maintainers handle it 2. Someone else coordinates the work across different modules 3. We remove the rule altogether

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #20) > Please don't land more rules at warning level (1), they just create noise > and confusion on try. Either off (0) or error (2) only. Ok ok, then I think error is better. (In reply to Lin Clark [:linclark] from comment #22) > Thanks for catching that janx. You're welcome! > I understand the concern about introducing a new warning, but introducing > this at 2 would mean that this patch has to do all of the work of changing > the order, which would be a coordination project that I don't have the focus > for atm. > > I see three options: > 1. We introduce it at 1 and let maintainers handle it > 2. Someone else coordinates the work across different modules > 3. We remove the rule altogether Well, if this rule has any value at all, this may be a good first bug to fix for someone interested in contributing to Firefox. Lin, would you be open to mentor this bug? If you've never done it, it's pretty easy: - Set the "mentor?" flag on yourself. - Add "[difficulty=medium][good first bug][lang=javascript]" to the whiteboard. - Briefly comment on what needs to be done to fix the bug. This will signal our community that this bug is mentored, and usually someone will show up to try and fix it. Here is an example: bug 1264929. Otherwise we can just drop the rule. I'm all for removing useless stuff.

Unfortunately I think mentoring the bug would take the same amount of focus (if not more depending on the experience level) so I would not be able to do it.

We now have a few React classes in Devtools, and we would like to make sure that their properties (attributes, methods, etc) are sorted in a way that make sense. The goal of this bug is to apply the attached patch "eslint.patch", change "react/sort-comp" error-level from "1" to "2", and fix any new error generated by: ./mach eslint devtools/ This can be done in one or several patches, depending on how many errors we fix. I can mentor this bug, so if you're interested in trying to fix it, please comment on the bug and set the "needinfo?" flag on ":janx". Please ask any questions if something isn't clear.

(Not adding "[difficulty=medium][good first bug][lang=javascript]" to the whiteboard yet, because the recursion of `./mach eslint devtools/` seems broken, see also bug 1264929.)

Here's a second patch that refactors react components to follow that new rule. All the errors thrown by ESLint were about displayName/propTypes order. I hope this is what you guys expected!

Comment on attachment 8751664 [details] [diff] [review] Refactor devtools react components Review of attachment 8751664 [details] [diff] [review]: ----------------------------------------------------------------- Works great, thank you Benoit! Please send both patches to try, and use the "checkin-needed" keyword when you're confident about the try results.

This is a rebased version of the second patch :)

And here are the tests! https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ac62ac3bc6f

https://hg.mozilla.org/integration/fx-team/rev/f27d4b4d5154 https://hg.mozilla.org/integration/fx-team/rev/0033e0f0a351

https://hg.mozilla.org/integration/fx-team/rev/f27d4b4d5154 https://hg.mozilla.org/integration/fx-team/rev/0033e0f0a351

Patches in flight, thank you Benoit! And congratulations on your second[0] patch \o/ [0] Found your first patch here: https://github.com/mozilla/gecko-dev/commits/master?author=bchabod

https://hg.mozilla.org/mozilla-central/rev/f27d4b4d5154 https://hg.mozilla.org/mozilla-central/rev/0033e0f0a351